feat(sdk-node): wire up tracer_provider.sampler from declarative config#6847
Conversation
Adds getSamplerFromConfiguration() that reads tracer_provider.sampler from the config model and passes it to the TracerProvider in startNodeSDK(). Standard sampler variants (always_on, always_off, trace_id_ratio_based, parent_based with all sub-samplers) are applied. Experimental variants (jaeger_remote/development, probability/development, composite/development) warn and fall back to ParentBased(AlwaysOn). buildSamplerFromConfig() is unchanged for non-parent_based callers; it now distinguishes experimental variants from genuinely unknown keys in its warning message. Closes open-telemetry#6506
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6847 +/- ##
==========================================
+ Coverage 95.56% 95.58% +0.01%
==========================================
Files 386 388 +2
Lines 13088 13159 +71
Branches 2998 3002 +4
==========================================
+ Hits 12508 12578 +70
- Misses 580 581 +1
🚀 New features to boost your workflow:
|
trentm
left a comment
trentm
left a comment
There was a problem hiding this comment.
Thanks. Looks good. See nit comment below.
A side note on my intent for the utility functions for the "create" step (creating SDK things from the ConfigurationModel). This is an eventual goal for internal design, not something to block this PR. I don't want to block PRs like this on my fledgling plan. Initial notes at #6785
- these utility functions are called
create<SDK Thing>FromConfig - they live in src/create-from-config.js
- they take specific
*ConfigModelarguments. (E.g. for this PR I'd havecreateSamplerFromConfig(sampler?: SamplerConfigModel): Sampler | undefined) - they throw if unable to create the thing described by the config
- they use
checkConfigUse()as appropriate
Remove the EXPERIMENTAL_SAMPLER_KEYS list and the dedicated warn branch. Unrecognized sampler types (including known experimentals like jaeger_remote/development) now fall through to a single diag.warn + ParentBased(AlwaysOn) default. Saves a maintenance touch point against the upstream schema and aligns with open-telemetry#6785's direction.
|
This PR has review comments. Review suggestions, whether from maintainers or automated reviewers, aren't always correct or required. Please evaluate each comment on its merits, then make sure each thread has a clear outcome. For example, link to the commit if you applied a suggestion, explain why it wasn't applied, or ask a follow-up question. Automation flags a PR for human review once every review thread has a reply or is marked as resolved. Status across open PRs is visible on the pull request dashboard. |
github-project-automation
Bot
moved this from In Progress
to Done
in Declarative Configuration: JavaScript
2 participants
Which problem is this PR solving?
tracer_provider.samplerin a declarative-config YAML is parsed and validated today but never applied to the SDK sostartNodeSDK()falls back to the default sampler regardless of what the YAML asks for. This is the last gating wiring gap for declarative config in JS.Fixes #6506.
Short description of the changes
getSamplerFromConfiguration(config)helper insdk-node/src/utils.tsthat returnsundefinedwhen no sampler is configured (lettingTracerProvideruse its default) or builds aSamplervia the existingbuildSamplerFromConfigotherwise.start.tsnow passes the resolved sampler toTracerProviderand drops theTODO (6506)comment.buildSamplerFromConfigdistinguishes experimental variants (jaeger_remote/development,probability/development,composite/development) from truly unknown keys in its fallback warning. Both still fall back toParentBased(AlwaysOn).getSamplerFromConfigurationand for the experimental-variant warn path.Type of change
How Has This Been Tested?
npm test- 234 tests pass, including new coveragenpm run lintcleanOut of scope
opentelemetry-sdk-trace-base/src/config.ts(loadDefaultConfig/buildSamplerFromEnv) is the env-var path used by the legacyBasicTracerProvidershim. It's still load-bearing for users not going throughstartNodeSDK()and is publicly exported. Rewriting it to use the configuration package is a separate breaking change and isn't required to unblock declarative-config.